-
Notifications
You must be signed in to change notification settings - Fork 490
Improvements to dynamic client registration #609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improvements to dynamic client registration #609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. It's a little odd for me to see the Authorization Server set up to support dynamic client registration but still require a secret. If you can securely distribute a secret with your application, why not just pre-register the application with a known client ID and skip DCR altogether?
Nonetheless, I see that Keycloak does require an initial access token for DCR by default and RFC 7591 does talk about the possibility of the client registration endpoint requiring an initial access token, and I don't see the harm in supporting it as long as we move the option into a ClientOAuthOptions.DynamicClientRegistration
sub property as not to confuse people.
Can you add some tests for this? You can look at AuthTests.cs and AuthEventTests.cs for examples.
@localden Do you have any thoughts on this?
src/ModelContextProtocol.Core/Authentication/ClientOAuthOptions.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.Core/Authentication/ClientOAuthProvider.cs
Outdated
Show resolved
Hide resolved
src/ModelContextProtocol.Core/Authentication/OAuthClientType.cs
Outdated
Show resolved
Hide resolved
@halter73 Thanks for the review, I'm not too familiar with dynamic client registration, but like you say it's the default configuration in Keycloak, so I tried to get it working in that configuration. I agree that pre-registering a client makes a lot more sense than DCR and is also easier, but the problem is that not all MCP clients support this. For example vscode has built-in support for microsoft and github authentication, but only supports DCR for third party authorization servers, so there's no way for me to pass the client id of the pre-configured client. I'm not sure if this has already been adressed in the 1.102 update, because when I tried testing it I ran into this issue: microsoft/vscode#255275 |
The problem is that clients like VS Code are likely not going to support DCR with an initial access token for the same reason that they do not support using a preregistered client ID and client secret which is that it requires additional configuration. If VS Code ever adds the ability to specify the DCR initial access token via config, why wouldn't they also add support for specifying a client ID and client secret for your preregistered client? |
@halter73 I think I've addressed all your feedback now, let me know if there's anything else you'd like to see changed. For me this change doesn't have much priority anymore since pre-registering a client and passing its client id is a better solution than DCR. This has also been made possible in the latest vscode insiders build and I finally got it working that way now. |
@halter73 |
- Make client_name optional since it's only RECOMMENDED by https://www.rfc-editor.org/rfc/rfc7591
Thanks @S-Luiten! |
Several improvements to dynamic client registration:
Motivation and Context
How Has This Been Tested?
This has been tested with an MCP server and MCP client both running locally, using keycloak as authorization server.
Breaking Changes
None
Types of changes
Checklist
Additional context
The default OAuth client type has been set to confidential so it's not a breaking change, but public might make more sense as the default?